Modular import/export taxonomy tags - #64
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @ChrisChV! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
4b9d584 to
b056500
Compare
| return str(error) | ||
|
|
||
| # Or return the plan | ||
| return dsl.plan() |
There was a problem hiding this comment.
Nit: It makes the type inference less useful if a function returns different data types in different situations... here it can return errors, a bool (from .plan()) or a plan. So if mypy is working correctly, you technically have to handle all three cases every time you call this function to avoid a type warning.
Instead, I'd recommend splitting plan() and execute() into separate functions, or making this combined one so that it always return a tuple like (plan: TagImportDSL, executed: bool). For errors you can raise them as an exception instead of an alternate return value.
- Added Parser, JSONParser and CSVParser classes to validate file and tags format - Tests added
1aa130e to
4dc7c6f
Compare
| Read a .json file and validates the root structure of the json | ||
| """ | ||
| file.seek(0) | ||
| tags_data = json.load(file) |
There was a problem hiding this comment.
Is it worth checking if the input is a valid JSON to avoid an exception? Will someone up in the stack call stack handle this?
| tags_data = json.load(file) | |
| try: | |
| tags_data = json.load(file) | |
| except json.JSONDecodeError as e: | |
| return None, [ | |
| InvalidFormat( | |
| tag=None, | |
| format=cls.format.value, | |
| message=str(e) | |
| ) | |
| ] | |
There was a problem hiding this comment.
Any errors occurred during the import are handled and logged in the import task. But yes, I agree with you that this verification should be more explicit.
|
|
||
| # Checks that exists only one tag | ||
| if not _check_unique_import_task(taxonomy): | ||
| raise ValueError( |
There was a problem hiding this comment.
In the future, we may need a function to force clean the status of an import task to avoid a lock. Maybe we end up with an invalid state (a task not in SUCCESS nor ERROR) caused by a server crash or an unforeseen condition that prevents us from doing further imports.
I don't think this is a problem for the MVP.
There was a problem hiding this comment.
Good catch! I added a list of TODOs on the docstring with this case
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Hey, this is very nice! Some comments/questions but I hope they're fairly minor.
|
@ChrisChV 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
6 participants
Description
Implements import_tags and export_tags functions
A modular implementation has been followed for both functionalities. You can see more about this here
Supporting information
Closes openedx/modular-learning#64
Testing instructions
make requirementsManual testing is possible but not very useful, since we don't yet have views for these models and APIs.
But you can check that the added tests cover the expected import actions and export outputs, and that tests are still covering 100% of the openedx_tagging module.